New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MRG] Fix LassoCV cross validation split() call #8973
Conversation
This test failed, but it seems is not related to this PR. |
|
||
pipe = make_pipeline( | ||
StandardScaler(), | ||
LassoCV(cv=KFold(n_splits=5)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test would fail on master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be StratifiedKFold
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, @raghavrv! My bad at pasting.
Fixed that, although I could not run tests locally today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small change in the tests... Otherwise good for merge
|
||
pipe = make_pipeline( | ||
StandardScaler(), | ||
LassoCV(cv=KFold(n_splits=5)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be StratifiedKFold
...
def test_lasso_cv_with_some_model_selection(): | ||
from sklearn.pipeline import make_pipeline | ||
from sklearn.preprocessing import StandardScaler | ||
from sklearn.model_selection import KFold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StratifiedKFold
LGTM, thanks |
* Fixing cross validation split call in LassoCV * Non-regression test for LassoCV cv.split check * Fix typo KFold->StratifiedKFold
Thank you for your help! Learnt a lot doing this, and happy for detecting the problem issue 8971 is going to solve. |
* Fixing cross validation split call in LassoCV * Non-regression test for LassoCV cv.split check * Fix typo KFold->StratifiedKFold
* Fixing cross validation split call in LassoCV * Non-regression test for LassoCV cv.split check * Fix typo KFold->StratifiedKFold
* Fixing cross validation split call in LassoCV * Non-regression test for LassoCV cv.split check * Fix typo KFold->StratifiedKFold
* Fixing cross validation split call in LassoCV * Non-regression test for LassoCV cv.split check * Fix typo KFold->StratifiedKFold
* Fixing cross validation split call in LassoCV * Non-regression test for LassoCV cv.split check * Fix typo KFold->StratifiedKFold
* Fixing cross validation split call in LassoCV * Non-regression test for LassoCV cv.split check * Fix typo KFold->StratifiedKFold
* Fixing cross validation split call in LassoCV * Non-regression test for LassoCV cv.split check * Fix typo KFold->StratifiedKFold
Reference Issue
Fixes #8426
What does this implement/fix? Explain your changes.
Added the missing
y
parameter that was causing the errorTypeError: split() missing 1 required positional argument: 'y'
, and its non-regression test.Any other comments?
Will be positively influenced by issue #8971.